-
Notifications
You must be signed in to change notification settings - Fork 27
compliance/sancation_list_update_centralization #45
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
compliance/sancation_list_update_centralization #45
Conversation
d28123d to
3db7903
Compare
| @@ -0,0 +1,230 @@ | |||
| package Data::Validate::Sanctions::Redis; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest naming it Data::Validate::Sanctions::Storage instead of Data::Validate::Sanctions::Redis. Redis is just a system that we are using, we may change this in future
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This module is exclusively created for redis storage. It relies on hmget and hmset calls and cannot be written without references to Redis. A better alternative IMHO is to use the parent class Data::Validate::Sactions as a Factory that can create an object of this subclass with a code like this:
my $validator = Data::Validate:::Sanctions->new(storage => 'redis`, connection => $redis);
…hub.com:mat-fs/perl-Data-Validate-Sanctions into compliance/sancation_list_update_centralization
.circleci/config.yml
Outdated
| - run: | ||
| name: Install Redis server | ||
| command: | | ||
| apt-get install -y redis && apt-get install -y redis-server |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you need install only redis-server
and if you want to install both , you can combine them:
| apt-get install -y redis && apt-get install -y redis-server | |
| apt-get install -y redis-server redis |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Changes
Outdated
| Revision history for Data-Validate-Sanctions | ||
|
|
||
| {{$NEXT}} | ||
| 0.14 2022-09-13 13:55:00 CST |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line can & should be removed. dzil release will add it automatically
| } | ||
|
|
||
| sub get_sanction_file { | ||
| $sanction_file //= _default_sanction_file(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will be better to use state here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in some cases, the state declaration does not apply because we are using the internal scope of the variable in the set_sanction_file and the get_sanction_file subroutines.
https://redmine.deriv.cloud/issues/70209